Skip to content

[rate-limit-controller] Fix and improve typing, remove any usage#3963

Merged
MajorLift merged 16 commits into
mainfrom
240222-rate-limit-controller-fix-any
Mar 7, 2024
Merged

[rate-limit-controller] Fix and improve typing, remove any usage#3963
MajorLift merged 16 commits into
mainfrom
240222-rate-limit-controller-fix-any

Conversation

@MajorLift
Copy link
Copy Markdown
Contributor

@MajorLift MajorLift commented Feb 22, 2024 •
edited
Loading

Explanation

  • Fix all any usage and more typing fixes and improvements.
  • Define RateLimitedApiMap, RateLimitedRequests for readability and clarity.
  • Rename types (this can be reverted if it's disruptive to any downstream package):
    • GetRateLimitState -> RateLimitControllerGetStateAction.
    • RateLimitStateChange -> RateLimitControllerStateChangeEvent.
    • CallApi -> RateLimitControllerCallApiAction.

References

Changelog

@metamask/rate-limit-controller

Added

  • Add and export types RateLimitedApiMap, RateLimitedRequests. (#3963)
    • RateLimitedApiMap represents the type of the RateLimitedApis generic parameter used throughout the controller.
    • RateLimitedRequests represents the type of the request property of RateLimitState.

Changed

  • BREAKING: Rename types to align with conventions followed by other controllers. (#3963)
    • GetRateLimitState is now RateLimitControllerGetStateAction.
    • RateLimitStateChange is now RateLimitControllerStateChangeEvent.
    • CallApi is now RateLimitControllerCallApiAction.
  • Add @metamask/utils ^8.3.0 as a dependency. (#3963)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift force-pushed the 240222-rate-limit-controller-fix-any branch from 2a01660 to 467f92a Compare February 22, 2024 21:42
@MajorLift MajorLift changed the base branch from main to 240221-base-controller-type-fixes February 22, 2024 21:43
@MajorLift MajorLift force-pushed the 240222-rate-limit-controller-fix-any branch from 467f92a to 380371b Compare February 22, 2024 22:37
@MajorLift MajorLift changed the title [rate-limit-controller] WIP [rate-limit-controller] Fix and improve typing, remove any usage Feb 22, 2024
@MajorLift MajorLift force-pushed the 240221-base-controller-type-fixes branch from 10f68ab to b35b77a Compare February 23, 2024 04:32
@MajorLift MajorLift force-pushed the 240222-rate-limit-controller-fix-any branch 2 times, most recently from e9e54f7 to 0aed2b9 Compare February 23, 2024 23:08
@MajorLift MajorLift self-assigned this Feb 23, 2024
@MajorLift MajorLift added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Feb 23, 2024
@MajorLift MajorLift marked this pull request as ready for review February 23, 2024 23:14
@MajorLift MajorLift requested a review from a team as a code owner February 23, 2024 23:14
Mrtenz
Mrtenz previously approved these changes Feb 26, 2024
View reviewed changes
MajorLift
MajorLift commented Feb 26, 2024
View reviewed changes
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Copy link
Copy Markdown
Contributor Author

@MajorLift MajorLift Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args should represent the result of querying RateLimitedApis with the specific type passed in as the second argument, not all possible Api types stored in state.

keyof RateLimitedApis is only the constraint for the type param, not its type.

Copy link
Copy Markdown
Contributor

@mcmire mcmire Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch.

legobeat
legobeat reviewed Feb 27, 2024
View reviewed changes
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
@MajorLift MajorLift force-pushed the 240222-rate-limit-controller-fix-any branch from 0aed2b9 to b393656 Compare February 27, 2024 23:32
mcmire
mcmire reviewed Mar 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just had some minor comments.

Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
Copy link
Copy Markdown
Contributor

@mcmire mcmire Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch.

Comment thread packages/rate-limit-controller/src/RateLimitController.ts Outdated
@mcmire
Copy link
Copy Markdown
Contributor

mcmire commented Mar 5, 2024

@MajorLift I'm assuming we want to wait until the parent PR is merged before merging this, correct?

@MajorLift MajorLift requested a review from a team as a code owner March 6, 2024 15:54
Base automatically changed from 240221-base-controller-type-fixes to main March 7, 2024 18:24
@MajorLift MajorLift dismissed Mrtenz’s stale review March 7, 2024 18:24

The base branch was changed.

@MajorLift MajorLift force-pushed the 240222-rate-limit-controller-fix-any branch from 66e78b8 to 244325e Compare March 7, 2024 18:34
mcmire
mcmire approved these changes Mar 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

cryptodev-2s
cryptodev-2s approved these changes Mar 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MajorLift MajorLift merged commit 6263cca into main Mar 7, 2024
@MajorLift MajorLift deleted the 240222-rate-limit-controller-fix-any branch March 7, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcmire mcmire mcmire approved these changes

@Mrtenz Mrtenz Mrtenz approved these changes

@cryptodev-2s cryptodev-2s cryptodev-2s approved these changes

+1 more reviewer

@legobeat legobeat legobeat left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@MajorLift MajorLift

Labels

base-controller-breaking-changes team-wallet-framework Deprecated: Please use `team-core-platform` instead.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rate-limit-controller: Replace use of any with proper types (non-test files only)

5 participants

@MajorLift @mcmire @Mrtenz @cryptodev-2s @legobeat